ci: run arcup installer shell tests and shellcheck - #247

Ran this locally against current main — everything in the PR description checks out, including the parts that usually go wrong in Makefile/CI PRs:

Claims verified:

• grep -rn "arcup" .github/workflows/ Makefile → no matches on main, confirmed. The 25-assertion suite (arcup/test_arcup.sh) really does have zero CI coverage today.
• The suite passes from the repo root (25 ok lines, exit 0) — this matters because the Makefile invokes bash arcup/test_arcup.sh from the top level, and the script handles it correctly via its ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" resolution rather than assuming CWD. No hidden working-directory assumption.
• Applied the patch and ran make test-arcup: works, exit 0. The recipe lines are real tabs (checked the raw patch bytes — ^I, not spaces), so no "missing separator" surprise.
• The glob guard is right: [ -f "$$suite" ] || continue cleanly skips the unmatched arcup/*_test.sh literal instead of feeding it to bash, and || exit 1 inside the for gives the fail-fast behavior claimed (make runs the recipe with /bin/sh -e semantics only per-line, so the explicit exit is actually load-bearing here, not decorative).
• The actions/checkout pin df4cb1c0… is byte-identical to the SHA every existing job in ci.yml uses (v6.0.3), so it follows the repo's pin-by-SHA convention rather than introducing a floating tag.
• shellcheck isn't installed in my environment so I couldn't reproduce the shellcheck-clean claim locally — but the job doesn't install it either, which is fine: GitHub's ubuntu-latest runners ship shellcheck preinstalled. Worth one maintainer glance only if this repo ever moves to a slim self-hosted runner, where the step would fail on a missing binary rather than a lint error.

One timing note in this PR's favor: #243 (just opened) adds regression tests to this exact suite for a checksum-verification edge case — and they only ever run if something like this lands. The two PRs compose cleanly with no overlap: #243 touches arcup/arcup + test_arcup.sh, this one touches only Makefile + ci.yml, so they can merge in either order without conflict, and whichever merges second immediately benefits from the other.

Only real design question is the one the author already flagged — standalone job vs. folding into an existing one. Standalone seems right to me: the job has zero toolchain deps, so folding it into a Rust job would couple a ~1-minute shell check to a much heavier setup for no gain, and a dedicated job name makes an installer regression legible at a glance in the checks list.

Thanks for taking the time to run this locally — checking the raw patch
bytes for tabs and confirming the ROOT_DIR resolution are exactly the two
things I'd have wanted a second pair of eyes on.

On shellcheck: you're right that the job leans on ubuntu-latest shipping it
preinstalled. I left it implicit to keep the job dependency-free, but I'm
happy to add a command -v shellcheck || sudo apt-get install -y shellcheck
guard if maintainers would rather not depend on the runner image. Say the
word and I'll push it.

Good catch on #243 composing with this — worth noting the direction of the
dependency, too: the regression tests you added there don't run in CI at all
until something like this lands. Either merge order works, but #243's
coverage only becomes load-bearing once the suite is actually wired up.

And agreed on standalone vs folded. Still happy to fold it into an existing
job if a maintainer prefers fewer checks in the list.

On the shellcheck guard question — the repo's own CI answers it. Looking at ci.yml, every existing job explicitly provisions its non-default tooling: the Rust jobs run sudo apt-get install -y --no-install-recommends libclang-dev zlib1g-dev before setup-rust-toolchain, Node goes through pinned actions/setup-node, Solidity through foundry-toolchain, protobuf through buf-action. Nothing in the file leans on the runner image for a tool the job actually depends on. So an explicit install isn't extra caution — it's matching the established convention; the current implicit dependency is the one thing in the job that deviates.

One nuance worth deciding deliberately rather than by accident: the two sources ship different shellcheck versions. The ubuntu-latest image bundles a recent release, while apt-get install shellcheck on noble resolves to the distro's older 0.9.x. Newer shellcheck versions add checks over time, so which binary runs determines which warning set gates the repo. That makes the command -v shellcheck || apt-get … guard the least predictable of the three options — it silently switches lint regimes the day the runner image changes, which is exactly when you don't want a surprise. Cleaner either way:

• Always-apt: sudo apt-get update && sudo apt-get install -y --no-install-recommends shellcheck — deterministic version from the distro, byte-consistent with how the Rust jobs phrase their apt line. Slightly older check set.
• Keep the image's binary, make it observable: no install step, but add shellcheck --version as the first line of the lint step, so when a future image bump introduces a new warning the CI log says why.

I'd take always-apt for the convention match and determinism; both scripts pass under 0.9.x-era checks anyway given they're clean under the newer one (the check set mostly grows, and the handful of removed/renumbered checks don't apply to code that's warning-free). Either choice is a one-line push — the job design itself doesn't move.

Went with always-apt — pushed. Convention argument is the persuasive one:
it was the only step in the job leaning on the image for a tool it
actually depends on, and matching the Rust jobs' phrasing costs nothing.

One correction on the premise, though, because it makes the choice
cheaper than you framed it: the two sources aren't currently different
versions. The ubuntu-24.04 image manifest lists shellcheck 0.9.0-1, and
apt on noble resolves to 0.9.0-1 — the image installs it from
noble/universe, so both paths land on the same package today.

That doesn't rescue the command -v guard; your argument against it
holds on its own terms, since it makes which binary runs an emergent
property of the image rather than a decision. But it does mean always-apt
costs nothing in check coverage right now — it isn't trading newer checks
for determinism, it's pinning what we're already getting before the two
can drift.

You're right, and I checked it against the source rather than taking either of our words for it: the actions/runner-images Ubuntu 24.04 manifest lists shellcheck 0.9.0-1 — the exact noble/universe package apt resolves to. My "image bundles a recent release" premise was stale; that was true of earlier image generations (which pulled standalone upstream binaries), but the 24.04 image gets it from the distro. Thanks for pinning that down instead of letting it slide — your framing is the accurate one: always-apt isn't trading check coverage for determinism today, it's freezing the current behavior before the two sources can drift.

Verified the push (5aa36a5a): the install line reads sudo apt-get update && sudo apt-get install -y --no-install-recommends shellcheck — byte-consistent with the Rust jobs' apt phrasing, and ordered before the lint step. With that, the one deviation from repo convention I'd flagged is gone.

Where this leaves the PR from my side: every claim in the description verified locally (suite passes from repo root, real tabs, load-bearing || exit 1, checkout SHA matches convention), the shellcheck provisioning now matches how every other job in ci.yml treats its tooling, and the composition story is settled — #243's regression tests activate the moment this lands, and #249 was rescoped to the finalize-release half at different anchors in both files, so there's no longer any overlapping claim on the arcup ground. Nothing outstanding that I can see; ready for maintainer review.